Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop fd acking in sidecar transport #481

Merged
merged 2 commits into from
Jun 10, 2024
Merged

Drop fd acking in sidecar transport #481

merged 2 commits into from
Jun 10, 2024

Conversation

bwoebi
Copy link
Contributor

@bwoebi bwoebi commented Jun 10, 2024

This has effectively been never working - it probably got added in long times past, before unidirectional sending was added. There's no ack, and thus it would simply leak and stay unclosed forever.

And the functionality is simply not needed. It's not like linux would close file descriptions as long as they're referenced in sendmsg buffers.

This has effectively been never working - it probably got added in long times past, before unidirectional sending was added.
There's no ack, and thus it would simply leak and stay unclosed forever.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi requested a review from a team as a code owner June 10, 2024 16:08
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.17%. Comparing base (29c7824) to head (8726804).

Current head 8726804 differs from pull request most recent head 4a50a84

Please upload reports for the commit 4a50a84 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #481      +/-   ##
==========================================
- Coverage   69.18%   69.17%   -0.01%     
==========================================
  Files         195      195              
  Lines       26144    26109      -35     
==========================================
- Hits        18087    18061      -26     
+ Misses       8057     8048       -9     
Components Coverage Δ
crashtracker 17.40% <ø> (ø)
datadog-alloc 98.76% <ø> (ø)
data-pipeline 51.30% <ø> (ø)
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 86.43% <ø> (ø)
ddcommon-ffi 74.15% <ø> (ø)
ddtelemetry 56.11% <ø> (ø)
ipc 84.61% <100.00%> (+0.07%) ⬆️
profiling 77.81% <ø> (ø)
profiling-ffi 59.53% <ø> (ø)
serverless 0.00% <ø> (ø)
sidecar 36.25% <ø> (ø)
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 69.36% <ø> (ø)
trace-normalization 97.79% <ø> (ø)
trace-obfuscation 95.74% <ø> (ø)
trace-protobuf 24.52% <ø> (ø)
trace-utils 91.03% <ø> (ø)

@bwoebi bwoebi merged commit 1aa5138 into main Jun 10, 2024
26 checks passed
@bwoebi bwoebi deleted the bob/drop-fd-acking branch June 10, 2024 16:46
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only repeat that it would have been nice with some comments trying to explain the whole machinery in here.

As for removing this ack thing, what does that mean for the rest of the convoluted code in PlatformHandle, and how do we test that properly?

@bwoebi
Copy link
Contributor Author

bwoebi commented Jun 11, 2024

I haven't written that code, but the primary issue is that, if a write couldn't be sent through (if buffer full), we'll have to retry the write. The rust async writes machinery will retry the write with a given buf then. But it doesn't have any idea of the adjacent file descriptions sent. So we'll have to store these separately and try sending them again when possible.
As Linux doesn't allow sending just file descriptors, it requires a non-empty accompanying message, we have to mix in the fd sending with the writes.

Also, we rely on in-order message delivery. So, we'll just buffer file descriptions until we know they were sent. (and may send file descriptions before the message they're actually attached to) The receiver side will then just pop handles as soon as a message expecting a file description is received.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants